Migrate sampler to non-contrib SDK incubator implementation#1091
Migrate sampler to non-contrib SDK incubator implementation#1091anuraaga wants to merge 1 commit into
Conversation
📋 ChangelogGenerated changelog entry for prs:
- "1091"
type: enhancement
products:
- product: edot-java
lifecycle: ga
title: Migrate sampler to non-contrib SDK incubator implementation |
📝 WalkthroughWalkthroughThe changes replace the consistent sampling library with the composite sampler framework. The 🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Review rate limit: 0/1 reviews remaining, refill in 60 minutes.Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
custom/src/main/java/co/elastic/otel/compositesampling/DynamicCompositeParentBasedTraceIdRatioBasedSampler.java (1)
54-57:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winFix the recursive description implementation.
getDescription()callsINSTANCE.getDescription(), which recurses on the same singleton and will overflow the stack as soon as the SDK asks for a description.Suggested fix
`@Override` public String getDescription() { - return INSTANCE.getDescription(); + return delegate.getDescription(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@custom/src/main/java/co/elastic/otel/compositesampling/DynamicCompositeParentBasedTraceIdRatioBasedSampler.java` around lines 54 - 57, The getDescription method currently calls INSTANCE.getDescription(), causing infinite recursion; change it to return the description of the underlying/wrapped sampler used by this composite (not the singleton INSTANCE). In other words, inside DynamicCompositeParentBasedTraceIdRatioBasedSampler.getDescription() return the wrapped sampler's description (e.g., wrappedSampler.getDescription() or delegateSampler.getDescription() — whatever field holds the underlying sampler in this class) or a fixed descriptive string for this composite, instead of delegating to INSTANCE.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In
`@custom/src/main/java/co/elastic/otel/compositesampling/DynamicCompositeParentBasedTraceIdRatioBasedSampler.java`:
- Around line 54-57: The getDescription method currently calls
INSTANCE.getDescription(), causing infinite recursion; change it to return the
description of the underlying/wrapped sampler used by this composite (not the
singleton INSTANCE). In other words, inside
DynamicCompositeParentBasedTraceIdRatioBasedSampler.getDescription() return the
wrapped sampler's description (e.g., wrappedSampler.getDescription() or
delegateSampler.getDescription() — whatever field holds the underlying sampler
in this class) or a fixed descriptive string for this composite, instead of
delegating to INSTANCE.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: 081d07c9-6e2a-4820-8ae1-3c72eb894023
📒 Files selected for processing (2)
custom/build.gradle.ktscustom/src/main/java/co/elastic/otel/compositesampling/DynamicCompositeParentBasedTraceIdRatioBasedSampler.java
💤 Files with no reviewable changes (1)
- custom/build.gradle.kts
anuraaga
added
changelog:skip
changelog:upstream-update
and removed
changelog:skip
labels
|
there's no need for this. we'll migrate the entire dynamic control to the contributed implementation when that's available, then this sampler gets removed, so there's no point in doing this in the interim |
|
@jackshirazi - Just to confirm what do you mean by that? This is the implementation I wrote based on the spec vs contrib which was a pre-spec POC. Isn't it good to switch to the maintained implementation? I mean I can send a PR to remove the contrib implementation which I think would be merged - would it help? 😉 |
|
This agent will move to use the dynamic control extension currently being added to contrib. That dynamic control extension already uses the SDK incubator sampler. So applying this PR would soon be pointless, so we'll avoid that additional work for now. Thanks for the contribution though, it would be right if we weren't aiming to switch relatively soon |
2 participants
No description provided.